Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call selectedUnitsHandler.RemoveUnit when a unit is killed. #1824

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saurtron
Copy link
Collaborator

@saurtron saurtron commented Dec 17, 2024

Work Done

  • Make Unit.cpp call selectedUnitsHandler::RemoveUnit on unit::ForcedUnitKill.

Remarks

  • Seems just an overlook
  • This is a real issue because api users get confused by units still in selection even after UnitDestroyed called

How to test

  • Get BAR branch from https://github.com/saurtron/Beyond-All-Reason/tree/test-selection-destroy
    • Could make a better test, but this is the one I used to find the issue
  • Start skirmish in neverend game end mode
  • Start game
  • Run /runtests attackrange
    • You might need to run this several times, but finally the assertion will trigger
    • gui_attackrange_gl4.lua has an issue where GetSelection still returns dead units, thus corrupting its logic
    • Might be other affected lua code in the wild not sure

@saurtron saurtron added the bug Something isn't working label Dec 17, 2024
@sprunk
Copy link
Collaborator

sprunk commented Dec 17, 2024

Engine should leave decisions to Lua, especially when it's trivial to do so there. What if I want to keep a unit with long death animation selected? Just deselect the unit in Lua UnitDestroyed.

@saurtron
Copy link
Collaborator Author

saurtron commented Dec 17, 2024

Engine should leave decisions to Lua, especially when it's trivial to do so there. What if I want to keep a unit with long death animation selected? Just deselect the unit in Lua UnitDestroyed.

This is causing real problems and very difficult to debug too.

@saurtron
Copy link
Collaborator Author

Just deselect the unit in Lua UnitDestroyed.

For your use case they can reselect in SelectionChanged. Imo it's much more niche and who knows how many widgets or gadgets have the issue this fixes.

@saurtron
Copy link
Collaborator Author

@sprung see the above issue from bar to see how easy it can be to fix bugs caused by this behaviour.

@sprunk
Copy link
Collaborator

sprunk commented Dec 17, 2024

  1. use this https://github.com/beyond-all-reason/Beyond-All-Reason/tree/sprunk-patch-2
  2. deselecting units doesn't solve any bug because you can just reselect the unit, at best it makes them harder to reproduce. Fix your gameside gadget. I would expect the issue to boil down to using UnitDestroyed where RenderUnitDestroyed is supposed to be used.

@saurtron
Copy link
Collaborator Author

saurtron commented Dec 17, 2024

2. deselecting units doesn't solve any bug because you can just reselect the unit, at best it makes them harder to reproduce. Fix your gameside gadget. I would expect the issue to boil down to using UnitDestroyed where RenderUnitDestroyed is supposed to be used.

Man, when a unit is killed and UnitDestroyed is called it should be removed automatically from selection, that's the most reasonable behaviour. Why would you reselect it? If you do of course you're on your own with all the problems you would cause in other places. I think that's totally reasonable.

Also I already noted this can result in difficult to track bugs it already cost me hours just to find what was going on, and also possibly other devs as well. Totally the last thing I expected was for the unit to still be there at the selection after I got it removed from visibleUnits and got UnitDestroyed too.

It's removed from basically everywhere.

@WatchTheFort
Copy link
Member

Having a destroyed unit selected does not make semantic sense.

@sprunk
Copy link
Collaborator

sprunk commented Dec 17, 2024

Having a destroyed unit selected does not make semantic sense.

It does at an engine level. But a game could disagree and it could make no sense within its design, in which case it is free to make such units unselectable via 1 line of Lua.

should be removed automatically from selection, that's the most reasonable behaviour.

The most reasonable behaviour is for the engine to let games decide on their own UI design via Lua, and facilitating that design's implementation in the form of "do X" rather than "work around the engine implementing some specific behaviour X".

If you do of course you're on your own with all the problems you would cause in other places.

There aren't actually any problems inherent to selecting dead units. BAR just has some broken wupget that handles it incorrectly, fix it (or use the 1-liner to work around it).

It's removed from basically everywhere.

Not from the places relevant here. Check RenderUnitDestroyed, see when it runs, think on why it exists and how it might help with the rendering issues you're having.

@saurtron
Copy link
Collaborator Author

saurtron commented Dec 17, 2024

It does at an engine level. But a game could disagree and it could make no sense within its design, in which case it is free to make such units unselectable via 1 line of Lua.

In the spirit of the "reasonable defaults" I think we should be able to agree the reasonable behaviour is to have them unselected, and then having games wanting otherwise to override that in some way. Otherwise we're just forcing games to workaround broken behaviour.

Actually I'm pretty sure the games must already be working around that since otherwise there would be more problems with all those selected but dead units.

Not from the places relevant here. Check RenderUnitDestroyed, see when it runs, think on why it exists and how it might help with the rendering issues you're having.

Could be to workaround some other broken behaviour, but can't be sure tbh.

@sprunk
Copy link
Collaborator

sprunk commented Dec 17, 2024

Actually I'm pretty sure the games must already be working around that since otherwise there would be more problems with all those selected but dead units.

A good topic for you to investigate.

RenderUnitDestroyed could be to workaround some other broken behaviour, but can't be sure tbh.

Another good topic to investigate.

Comment on lines +484 to +485
if (isSelected)
selectedUnitsHandler.RemoveUnit(this);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (isSelected)
selectedUnitsHandler.RemoveUnit(this);
/* This is a hack and should be done gadget-side,
* because else it's a hassle to keep the unit selected
* if you'd rather that was the behaviour. */
if (isSelected)
selectedUnitsHandler.RemoveUnit(this);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants